[SPARK-59696][SQL] Skip the redundant second modulo in integral MathUtils.pmod for positive divisors by david-mollitor-db · Pull Request #58906 · apache/spark

david-mollitor-db · 2026-09-18T03:18:02Z

What changes were proposed in this pull request?

The integral MathUtils.pmod overloads (Int, Long, Byte, Short) compute the positive
remainder as:

val r = a % n
if (r < 0) (r + n) % n else r

The trailing % n is redundant only when the divisor n > 0: a % n has magnitude strictly
less than |n|, so for r < 0 and n > 0, r + n already lies in [0, n) and the second modulo
is a no-op. For n < 0, r + n can fall below n and still needs reducing, so the % n must be
kept. This PR skips the modulo exactly where it is redundant and retains it otherwise:

val r = a % n
if (r >= 0) r
else if (n > 0) r + n
else (r + n) % n

Note: an earlier revision of this PR dropped the % n unconditionally. That is not
behavior-preserving. Pmod passes the divisor straight to MathUtils.pmod with no sign
normalization, so a negative divisor reaches this code, and e.g. pmod(-3, -5) would change from
-3 to -8 -- an out-of-range value that also diverges from the (unchanged) decimal pmod path.
pmod is only guaranteed non-negative for a positive divisor; for a negative divisor it shares
the sign of the dividend, and that released behavior is preserved here.

The Float/Double overloads are left unchanged on purpose: floating-point rounding can round
r + n up to exactly n, and the extra % n folds that back to 0, so there it is a real
correctness guard rather than a no-op.

Why are the changes needed?

MathUtils.pmod backs the pmod SQL function and the per-row partition-id expression of
HashPartitioning (Pmod(Murmur3Hash(...), numPartitions)), so it runs once per row on the map
side of every hash-partitioned shuffle -- always with a positive divisor (numPartitions). Removing
the redundant modulo in that case takes an integer division off the hot path.

JMH microbenchmark of the three forms (16,384 pmod(int, int) calls per op, JDK 17, JMH 1.37,
2 forks x 5 iterations, AverageTime):

divisor `n`	OLD `(r+n)%n`	drop-all (rejected)	this PR (guarded)	guarded vs OLD
`8` (n > 0)	81.05 us/op	25.26 us/op	25.28 us/op	~3.2x faster
`200` (n > 0)	88.90 us/op	25.28 us/op	25.31 us/op	~3.5x faster
`-200` (n < 0)	87.46 us/op	25.28 us/op*	87.10 us/op	unchanged (correct)

For positive divisors the guarded form matches the unconditional drop (the if (n > 0) test is
free) and is ~3.2-3.5x faster than the original. For negative divisors it correctly keeps the
modulo, matching the original cost and result. (*the drop-all column at n < 0 is fast only because
it skips the required modulo and returns the wrong, out-of-range value.)

Does this PR introduce any user-facing change?

No. The result is identical to released Spark for every input, including negative divisors.

How was this patch tested?

New regression cases in ArithmeticExpressionSuite cover a negative dividend with a negative
divisor (e.g. pmod(-3, -5) == -3) -- the r < 0, n < 0 path that an unconditional drop would
break and that existing tests did not exercise.
The guarded form was cross-checked for exact equality with the original against an exhaustive
oracle: all byte and short (a, n) pairs, and all 2^32 int dividends across positive and
negative divisors, with zero mismatches.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Opus 4.8

dongjoon-hyun

For code change, it would be great if you can make a PR with the JIRA issue for trace-ability, @david-mollitor-db .

david-mollitor-db · 2026-09-21T19:09:43Z

Thank you for the review, @dongjoon-hyun, and my apologies for opening this without a JIRA ID — I'm a newcomer to Spark and still getting familiar with the contribution process.

I've filed SPARK-59696 and updated the PR title and commit message to reference it. Could you please take another look when you have a chance? Thank you for your patience.

dongjoon-hyun

Thank you for filing the JIRA issue and updating the PR, @david-mollitor-db .

The logic looks correct to me. For n > 0 and r < 0, r is in (-n, 0), so r + n is in (0, n) without overflow and the second % n is a no-op. The n < 0 path is identical to the existing code, and both eval and codegen go through the same MathUtils.pmod, so they stay consistent. Keeping the Float/Double overloads as-is also makes sense.

I have only a few minor comments.

The PR title, Remove redundant second modulo ..., seems to describe the earlier revision (unconditional removal). Since this PR skips it only for positive divisors, could you revise the title to match, e.g., Skip the redundant second modulo in integral MathUtils.pmod for positive divisors? It will be the commit title.
The existing comment above your new one says adjusted to share the sign of n, which is not accurate for a > 0, n < 0 (a positive r is returned as-is) and doesn't match your PR description (it shares the sign of the dividend). Since the new comment sits right next to it, it would be nice to fix it together.
(nit) The Byte/Short overloads use a one-line val result = ... form while Int/Long use the three-line if / else if / else form. It would be good to be consistent.

Let's wait for the CI results.

david-mollitor-db · 2026-09-21T19:17:08Z

One more note for the record, in the interest of transparency:

My initial implementation dropped the second % n unconditionally, and the entire existing unit-test suite still passed — none of the pre-existing cases exercised the negative-dividend / negative-divisor path (r < 0, n < 0), which is exactly where that shortcut is wrong (pmod(-3, -5) would return -8 instead of the released -3).

I've corrected the implementation to keep the modulo when the divisor is negative, and added regression cases in ArithmeticExpressionSuite for that path. These negative-divisor inputs are unlikely in practice — the primary internal caller, HashPartitioning, always passes a positive divisor (numPartitions) — but they're reachable through the pmod SQL function, so I added the coverage to lock in the existing behavior regardless.

Addressed.

…tils.pmod for positive divisors The integral `MathUtils.pmod` overloads (Int, Long, Byte, Short) compute `if (r < 0) (r + n) % n else r`, where `r = a % n`. When the divisor `n > 0`, a negative `r` lies in `(-n, 0)`, so `r + n` is already in `[0, n)` and the trailing `% n` is a no-op; it is skipped there. When `n < 0`, `r + n` can fall below `n` and still needs reducing, so the `% n` is retained: if (r >= 0) r else if (n > 0) r + n else (r + n) % n This is behavior-preserving for every input, including negative divisors. An unconditional drop of the `% n` would change e.g. `pmod(-3, -5)` from `-3` to `-8` (out of range) and diverge from the (unchanged) decimal `pmod` path. The guarded form was verified equivalent to the original against an exhaustive oracle: all byte and short (a, n) pairs, and all 2^32 int dividends across positive and negative divisors. Added regression cases in `ArithmeticExpressionSuite` for a negative dividend with a negative divisor -- the `r < 0`, `n < 0` path that an unconditional drop would break and that existing tests did not cover. The float/double overloads intentionally keep the second modulo: floating-point rounding can make `r + n` round up to exactly `n`, which `% n` folds back to 0. `pmod` backs the `pmod` SQL function and `HashPartitioning`'s per-row partition-id expression; for positive divisors this removes an integer division from the hot path (~3.3x faster on the changed branch in a JMH microbenchmark; numbers in the PR description). Co-authored-by: Isaac <no-reply@databricks.com>

david-mollitor-db · 2026-09-22T01:50:27Z

Thanks @dongjoon-hyun. Addressed all three: retitled to "Skip the redundant second modulo ... for positive divisors", corrected the comment above pmod (it's non-negative for n > 0 and shares the dividend's sign for n < 0), and made the Byte/Short overloads use the same three-line form as Int/Long. Rebased on master and re-pushed.

david-mollitor-db · 2026-09-28T17:09:26Z

Hi @dongjoon-hyun, gentle nudge on this one -- it should be ready for another look whenever you have a moment. Thanks again for your time!

The integral pmod overloads used a multi-line brace-less if/else if/else, which the Databricks Scala style guide flags (curly braces required unless a one-line if/else ternary). Restore the one-line ternary form to match the file's Float/Double overloads and the pre-existing style. No logic change. Co-authored-by: Isaac <no-reply@databricks.com>

dongjoon-hyun

Thank you for the update, @david-mollitor-db. I reviewed the latest commit (0f45189).

The optimization itself looks correct and behavior-preserving. The old and new integral bodies return identical results for all byte/short pairs and for 2e8 random int and long pairs each, plus MinValue/MaxValue edge values, and the one-line form compiles to the same bytecode as the previous revision. Most of the inline comments are about the new comments and tests rather than the code change itself.

The new n < 0 sentence in the header comment doesn't hold for Int/Long when r + n overflows, e.g. pmod(-1, Int.MinValue) == Int.MaxValue (L93). The overflow itself is pre-existing and silent even in ANSI mode, so it may deserve a separate JIRA (L104).
The new test comment says pmod is "only positive for a positive divisor", which the assertions right above it contradict (L585).
The new negative-divisor cases can't detect replacing (r + n) % n with r, because only overflow inputs distinguish the two (L586).
Optional: a branchless form for non-constant divisors (L109), Byte/Short delegation (L114), testNumericDataTypes (L588), Math.floorMod (L97), and the Float/Double +Infinity case (L92).

In addition,

The PR description, which becomes the commit message, still shows the previous three-line snippet and the review history ("an earlier revision ...", the "drop-all (rejected)" column). Could you update the snippet to the current one-line form and drop the history? Also, on the literal-divisor path (HashPartitioning), C2 already strength-reduces % numPartitions into a multiply/shift, so the gain there comes from the branchless sign fix-up rather than from taking an integer division off the hot path.
Outside of this diff (not blockers; they can be follow-ups):
- The user-facing docs still say "Returns the positive value of expr1 mod expr2" (arithmetic.scala#L1123, and the same wording in functions.scala, PySpark, and R), while pmod(-7, -3) returns -1.
- The Decimal path (arithmetic.scala#L1303 and its codegen) keeps the same redundant second remainder for n > 0, which is also an exact no-op for decimals.
- The Pmod.doGenCode comment "the same fixed algorithm for every primitive numeric type" (arithmetic.scala#L1242) is now slightly loose; "same semantics" would be more accurate.

Let's wait for the CI results.

dongjoon-hyun · 2026-09-28T20:04:39Z

  def floorMod(a: Long, b: Long): Long = withOverflow(Math.floorMod(a, b))

-  // Positive modulo (`pmod`): the remainder `a % n` adjusted to share the sign of `n`.
+  // Positive modulo (`pmod`): the remainder `a % n` shifted to be non-negative when the divisor


nit: this header also covers the Float/Double overloads (L100 mentions them), which return NaN for a +Infinity divisor and a negative finite dividend, e.g. pmod(-1.0, Double.PositiveInfinity) is (-1.0 + Inf) % Inf = NaN. It's pre-existing and minor, but you may want to scope the non-negative guarantee to finite inputs or to the integral overloads.

dongjoon-hyun · 2026-09-28T20:04:39Z


-  // Positive modulo (`pmod`): the remainder `a % n` adjusted to share the sign of `n`.
+  // Positive modulo (`pmod`): the remainder `a % n` shifted to be non-negative when the divisor
+  // `n > 0`; for `n < 0` the result instead shares the sign of the dividend `a`.


For Int/Long, this n < 0 statement doesn't hold when r + n overflows in the retained (r + n) % n (only possible for n < -2^30 on Int and n < -2^62 on Long). For example,

pmod(-1, Int.MinValue) == 2147483647

pmod(-2, -2147483647) == 0

pmod(-1073741824, -1073741825) == 1073741822

pmod(-1L, Long.MinValue) == Long.MaxValue

It does hold for Byte/Short (computed in Int) and for non-overflowing Int/Long inputs. Could you add an overflow caveat here, or limit the statement to the n > 0 guarantee? The PR description has the same sentence.

dongjoon-hyun · 2026-09-28T20:04:39Z

+  // The `r < 0` branch shifts the remainder by `n`. When `n > 0`, `r` lies in `(-n, 0)` so the
+  // shifted value `r + n` is already in `[0, n)` and the extra `% n` is a no-op -- it is skipped.


nit: for n > 0, the new fast path is exactly java.lang.Math.floorMod(a, n), whose result the JDK guarantees to be in [0, n) for a positive divisor (on JDK 17/21 it is also one % plus a conditional add). So this range argument could simply refer to that. It's only a wording suggestion; the current code is fine.

dongjoon-hyun · 2026-09-28T20:04:39Z

  def pmod(a: Int, n: Int): Int = {
    val r = a % n
-    if (r < 0) (r + n) % n else r
+    if (r >= 0) r else if (n > 0) r + n else (r + n) % n


This is pre-existing (the old body behaves identically), so it's not a regression of this PR. But since this line is rewritten: the retained (r + n) % n silently wraps around for Int when n < -2^30 (and for Long at L109 when n < -2^62), returning a non-negative value for a negative dividend.

SELECT pmod(-1, CAST(-2147483648 AS INT)) returns 2147483647, while BIGINT and DECIMAL return -1 for the same values, and the Byte/Short overloads return -1 for pmod(-1, MinValue).

pmod(-1073741824, -1073741825) == 1073741822, which is not even congruent to the dividend modulo n.

ANSI mode doesn't raise ARITHMETIC_OVERFLOW because failOnError only covers the zero divisor.

Without overflow, (r + n) % n == r, so else r would fix it. Since that changes released results for these inputs, a separate JIRA seems better than this PR.

dongjoon-hyun · 2026-09-28T20:04:39Z

  def pmod(a: Byte, n: Byte): Byte = {
    val r = a % n
-    if (r < 0) ((r + n) % n).toByte else r.toByte
+    (if (r >= 0) r else if (n > 0) r + n else (r + n) % n).toByte


Optional: a % n on Byte/Short is already Int arithmetic, so r + n can't overflow here and the (r + n) % n arm always equals r. So the Byte (here) and Short (L119) overloads could simply delegate to the Int one. It is exactly equivalent (checked for all byte and short pairs) and leaves one copy of the branch chain per width, which also keeps them consistent by construction.

def pmod(a: Byte, n: Byte): Byte = pmod(a.toInt, n.toInt).toByte def pmod(a: Short, n: Short): Short = pmod(a.toInt, n.toInt).toShort

Generated code keeps calling pmod(byte, byte) / pmod(short, short), so codegen is unaffected.

dongjoon-hyun · 2026-09-28T20:04:39Z

    checkEvaluation(Pmod(positiveInt, negativeInt), positiveInt)
    checkEvaluation(Pmod(positiveLong, negativeLong), positiveLong)
+    // Negative divisor (n < 0): `pmod` is only positive for a positive divisor, so these expected
+    // values are intentionally <= 0 (released behavior). They guard the r < 0, n < 0 path where


These cases only distinguish the retained (r + n) % n from the rejected r + n. Without overflow, (r + n) % n == r, so replacing that branch with plain r (which looks like an obvious simplification) would still pass all five cases while changing released results such as pmod(-1, Int.MinValue) (2147483647 -> -1) and pmod(-1L, Long.MinValue) (Long.MaxValue -> -1). checkConsistencyBetweenInterpretedAndCodegenAllowingException can't catch it either because both paths call MathUtils.pmod.

If exact compatibility is the intent, could you pin boundary cases like the following? Or this can be left to the follow-up that decides the overflow behavior (see L104).

checkEvaluation(Pmod(Literal(-1), Literal(Int.MinValue)), Int.MaxValue) checkEvaluation(Pmod(Literal(-1L), Literal(Long.MinValue)), Long.MaxValue)

dongjoon-hyun · 2026-09-28T20:04:39Z

+    checkEvaluation(Pmod(Literal(-3), Literal(-5)), -3)
+    checkEvaluation(Pmod(Literal(-7), Literal(-3)), -1)
+    checkEvaluation(Pmod(Literal(-3L), Literal(-5L)), -3L)
+    checkEvaluation(Pmod(Literal((-3).toShort), Literal((-5).toShort)), (-3).toShort)
+    checkEvaluation(Pmod(Literal((-7).toByte), Literal((-3).toByte)), (-1).toByte)


nit: these could go into the existing testNumericDataTypes { convert => ... } block at the top of this test, which would cover all 7 numeric types with both pairs. Currently Int has both pairs, Long/Short only (-3, -5), and Byte only (-7, -3).

checkEvaluation(Pmod(Literal(convert(-3)), Literal(convert(-5))), convert(-3)) checkEvaluation(Pmod(Literal(convert(-7)), Literal(convert(-3))), convert(-1))

…essions/ArithmeticExpressionSuite.scala Co-authored-by: Dongjoon Hyun <dongjoon@apache.org>

david-mollitor-db marked this pull request as draft September 20, 2026 15:01

david-mollitor-db force-pushed the pmod-drop-redundant-modulo branch from 6153f7c to a61d12a Compare September 20, 2026 18:35

david-mollitor-db marked this pull request as ready for review September 20, 2026 18:36

david-mollitor-db force-pushed the pmod-drop-redundant-modulo branch from a61d12a to d01a4a0 Compare September 21, 2026 02:25

dongjoon-hyun previously requested changes Sep 21, 2026

View reviewed changes

david-mollitor-db force-pushed the pmod-drop-redundant-modulo branch from d01a4a0 to 41489ef Compare September 21, 2026 19:02

david-mollitor-db changed the title ~~[MINOR][SQL] Remove redundant second modulo in integral MathUtils.pmod~~ [SPARK-59696][SQL] Remove redundant second modulo in integral MathUtils.pmod Sep 21, 2026

david-mollitor-db requested a review from dongjoon-hyun September 21, 2026 19:11

dongjoon-hyun reviewed Sep 21, 2026

View reviewed changes

david-mollitor-db force-pushed the pmod-drop-redundant-modulo branch from 41489ef to a157084 Compare September 22, 2026 01:48

david-mollitor-db changed the title ~~[SPARK-59696][SQL] Remove redundant second modulo in integral MathUtils.pmod~~ [SPARK-59696][SQL] Skip the redundant second modulo in integral MathUtils.pmod for positive divisors Sep 22, 2026

david-mollitor-db requested a review from dongjoon-hyun September 22, 2026 16:02

dongjoon-hyun reviewed Sep 28, 2026

View reviewed changes

Comment thread sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala Outdated

dongjoon-hyun reviewed Sep 28, 2026

View reviewed changes

Update sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expr…

827f745

…essions/ArithmeticExpressionSuite.scala Co-authored-by: Dongjoon Hyun <dongjoon@apache.org>

		// The `r < 0` branch shifts the remainder by `n`. When `n > 0`, `r` lies in `(-n, 0)` so the
		// shifted value `r + n` is already in `[0, n)` and the extra `% n` is a no-op -- it is skipped.

Conversation

david-mollitor-db commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

david-mollitor-db commented Sep 21, 2026

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

david-mollitor-db commented Sep 21, 2026

Uh oh!

david-mollitor-db commented Sep 22, 2026

Uh oh!

david-mollitor-db commented Sep 28, 2026

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

david-mollitor-db commented Sep 18, 2026 •

edited

Loading